[InputBase] Remove legacy lifecycle methods #13487
Conversation
WARNING: Fix might be considered breaking
| if (!prevProps.disabled && this.props.disabled) { | ||
| const { muiFormControl } = this.context; | ||
| if (muiFormControl && muiFormControl.onBlur) { | ||
| muiFormControl.onBlur(); |
There was a problem hiding this comment.
This is something that should go to the chopping block in a major. Why would someone put the component into the context if he then controls it via props on InputBase rather than props on `FormControl``
eps1lon
changed the title
| @@ -48,6 +48,13 @@ export const styles = { | |||
| * ⚠️ Only one input can be used within a FormControl. | |||
| */ | |||
| class FormControl extends React.Component { | |||
| static getDerivedStateFromProps(props, state) { | |||
There was a problem hiding this comment.
I think that it's the InputBase that should take care of it by calling handleBlur. Why do we need this logic?
There was a problem hiding this comment.
- <FormControl disabled={false}><InputBase /></FormControl>
+ <FormControl disabled={true}><InputBase /></FormControl>InputBase already calls handleBlur separately if it's own focused state changes.
There was a problem hiding this comment.
I don't follow, the disable change will propagate down to the input.
There was a problem hiding this comment.
Maybe we need an integration test? :)
There was a problem hiding this comment.
It's context not props propagation. If you think that some behavior was already covered feel free to add a test. Especially focused was not considered correctly.
But why should InputBase call onBlur in it's parent when the parent got changed and can take care of itself? No need to delegate that logic into the child.
There was a problem hiding this comment.
The form control should propagate the disabled state to the input with the context. The input can then collect it. This way, we can avoid logic duplication. I think that we should remove the old context usage at the same time this logic is very dependant on the context implementation. I think that it will be simpler and less error prone to do both at the same time.
Yeah, it would be great to have an integration tests. It's tricky. Well, I'm also fine with logic duplication if it's simpler.
There was a problem hiding this comment.
It's only logic duplication if you have a mental model with the old context and lifecycle methods. I don't think the discussion is helpful here. If you know how to access context in gDSFP or cDU I'd be happy to change it but I don't know how.
this logic is very dependant on the context implementation.
I can only work with what's been given to me. InputBase was always tightly coupled with the context.
There was a problem hiding this comment.
gDSFP or cDU I'd be happy to change it but I don't know how.
We can't, it's why I'm raising the new context api, where we would be injecting the context as properties.
oliviertassinari
added
new feature
Removes legacy lifecycle methods that would require legacy context API.
cWRP->gDSFP,cWU->cDU.Preface: Allowing users to handle inputs in a mix of context and props should not be allowed in future implementations. Either the Provider or the Component should be the source of truth.
There will be breakage if this component was handled in a mixed way between controlled via props vs controlled via context e.g.
This would previously switch the
formControlStatefrom not disabled to disabled because it only uses context if the prop is undefined. This would would triggersetState({ focused: false })incWRP. However we don't have access to context ingDSFPso now the input would still be focused andonBlurwill not fire.I see no other way forward and actually a fix. I hope nobody relied on this.
Other issues:
context.onBlue()won't get fired as well asgDSFPin theFormControllstate.focusedwon't be reset. Seems like we really need to split this component up.